-
Notifications
You must be signed in to change notification settings - Fork 197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Session replay improvements #9
Conversation
MikeShi42
commented
Sep 19, 2023
- Add explicit email filter
- Add empty replay message info
- Filter out empty replay sessions from the replay page as it's usually confusing (we may want to revisit using the replay table instead of the event stream table for filtering though - open to comments)
…ty replay search filter
🦋 Changeset detectedLatest commit: 3b4715f The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -1090,9 +1089,36 @@ export const getSessions = async ({ | |||
], | |||
); | |||
|
|||
const sessionsWithRecordingsQuery = SqlString.format( | |||
`WITH sessions AS (${sessionsWithSearchQuery}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: would be nice to merge queries for readablility (also make query parameterization migration a bit easier). but no big deal, I can clean it up later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case this query execution is conditional (as this is a slower path), unless you were thinking of having the original query CTE duplicated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I was thinking to have CTE inline basically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'ma merge this as-is right now, and we can probably revisit that later - likely we'll need to tune this due to perf anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm